Skip to content

feat(app): Added controller attributes #9745

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 10 commits into
base: 4.7
Choose a base branch
from
Open

feat(app): Added controller attributes #9745

wants to merge 10 commits into from

Conversation

lonnieezell
Copy link
Member

@lonnieezell lonnieezell commented Oct 5, 2025 •
edited
Loading

Description
This adds the ability to use attributes on controller classes or methods. The primary goal of this is to get us as close to feature-parity to auto-routing improved with the more robust route definitions. This adds provides the following 3 attributes that can be used by any controller (not just auto-routing):

  • Filter: applies any of the defined filters with optional parameters
  • Restrict: restrict the controller/method by environment, hostname, or subdomain
  • Cache: this is a new feature, but will automatically handle caching the results of the method server-side.

This also brings some of these behaviors closer to the location where they should be run, making it more obvious what's being processed where.

Examples:

#[Filter(by: 'group', having: ['admin', 'superadmin'])]
class AdminController extends BaseController
{
    #[Filter(by: 'permission', having: ['users.manage'])]
    public function users()
    {
        // Will have 'group' filter with ['admin', 'superadmin']
        // and 'permission' filter with ['users.manage']
    }
}
#[Restrict(environment: ['development', '!production'])]
class HomeController extends BaseController
{
    // Restrict access by hostname
    #[Restrict(hostname: 'localhost')]
    public function index()
    {
    }

    // Multiple allowed hosts
    #[Restrict(hostname: ['localhost', '127.0.0.1', 'dev.example.com'])]
    public function devOnly()
    {
    }

    // Restrict to subdomain, e.g. admin.example.com
    #[Restrict(subdomain: 'admin')]
    public function deleteItem($id)
    {
    }
}
class HomeController extends BaseController
{
    // Cache this method's response for 2 hours
    #[Cache(for: 2 * HOUR)]
    public function index()
    {
        return view('welcome_message');
    }

    // Custom cache key
    #[Cache(for: 10 * MINUTE, key: 'custom_cache_key')]
    public function custom()
    {
        return 'This response is cached with a custom key for 10 minutes.';
    }
}

Checklist:

  • Securely signed commits
  • Component(s) with PHPDoc blocks, only if necessary or adds value (without duplication)
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@lonnieezell lonnieezell requested a review from michalsn October 5, 2025 22:47
michalsn
michalsn reviewed Oct 6, 2025
View reviewed changes
Copy link
Member

@michalsn michalsn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks like a really nice feature to have in the framework.

system/Router/Attributes/Restrict.php
#[Attribute(Attribute::TARGET_CLASS | Attribute::TARGET_METHOD | Attribute::IS_REPEATABLE)]
class Restrict implements RouteAttributeInterface
{
private const TWO_PART_TLDS = [
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should extract this list into a separate config file. There are a lot more domains like this (e.g., com.pl, gov.pl, and regional ones like poznan.pl), and that's just from my country.

There's no point in trying to hardcode every possible combination here. Moving it into a config file would allow developers to add or override entries as needed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good call. i might take another look at better detecting subdomains. I just realized I forgot to look how the router was currently doing it like I was planning on. This doesn't feel wonderful but it does seem the most accurate way I could come up with at the time.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm looking in the right place, then it seems like the Router is doing this even simpler: https://github.com/codeigniter4/CodeIgniter4/blob/develop/system/Router/RouteCollection.php#L1665

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is doing it simpler. And only account for co.* doubles. Not great. I think what needs to happen is that after this is approved and merged we need to create a new method in the url_helper to detect subdomains, and use a config file for more robust double detections.

@michalsn michalsn added new feature PRs for new features 4.7 GPG-Signing needed Pull requests that need GPG-Signing labels Oct 6, 2025
@neznaika0
Copy link
Contributor

It would be worth combining together with https://github.com/kenjis/ci4-attribute-routes
You can improve a lot of things this way.

However, the use cases are unclear to me. I have never used cache or restrictions.

@lonnieezell
Copy link
Member Author

It would be worth combining together with https://github.com/kenjis/ci4-attribute-routes You can improve a lot of things this way.

I hadn't seen that one. That's an interesting use case! Since that's not a run-time use, though, it handles a different use case, so probably doesn't make sense to merge features. But thanks for sharing it! I need to play around with that at some point.

However, the use cases are unclear to me. I have never used cache or restrictions.

The cache attribute is an idea I've had for a while now to make it really simple to cache whole endpoints without all of the boilerplate. It never made sense on it's own, though. I recently starting thinking more about where CI fits in the marketplace, where it can be simpler, etc. and I think I may have led us a little astray when I pushed the route declaration files in 4.0 as the primary way to route. Kenjis auto-routing improved that they added is a great middle-ground for ease of use and security.

This was an attempt to get auto-routing to feature parity with route declaration files. Many of the other features are already handled by file-based auto-routing (like prefixes, namespaces, etc). Once I get to some doc rewrites that are coming up, I want to present auto-routing first, since it is easy to understand that mapping of files, I think. As a bonus - auto-routing should have higher performance, though I need to look at one thing with it that I know if.

As for usefulness of the features? I think they're pretty handy. I've occassionally built dev-only pages/tools and use the environment restrictions to do that. Caching can be a huge win. And if we want auto-routing to be the primary way, it really needed a nicer interface with the controller filters.

@neznaika0
Copy link
Contributor

I remember being told that it's better to have a backup value. In case of compatibility.
See in code:
$oldFilterOrder = config(Feature::class)->oldFilterOrder ?? false;
But otherwise, you don't have to compare true/false, it's better to ! config(Routing::class)->useControllerAttributes

michalsn
michalsn reviewed Oct 7, 2025
View reviewed changes
@lonnieezell
Copy link
Member Author

@michalsn If you see the last few commits, GPG signing is enabled. I believe the check is failing because the earlier commits were not signed. do we need to deal with that somehow or will an override be ok?

@michalsn
Copy link
Member

michalsn commented Oct 7, 2025

@lonnieezell Yeah, we require all commits to be signed. You can sign old commits by following these instructions: https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#gpg-signing-old-commits

@lonnieezell lonnieezell force-pushed the autoroute-attributes branch from c37d04a to 58a9eba Compare October 7, 2025 20:41
@lonnieezell
Copy link
Member Author

@michalsn

@lonnieezell Yeah, we require all commits to be signed. You can sign old commits by following these instructions: https://github.com/codeigniter4/CodeIgniter4/blob/develop/contributing/workflow.md#gpg-signing-old-commits

Done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michalsn michalsn michalsn left review comments

Assignees
No one assigned
Labels
4.7 GPG-Signing needed Pull requests that need GPG-Signing new feature PRs for new features
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lonnieezell @neznaika0 @michalsn